layout_catalog#6290
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a couple of checklist-level issues that are considered critical blockers for this task. First, the header border color #e5e5e5 is used as a raw hex value in your styles (e.g. _header.scss, index.scss), but the checklist requires that special colors be stored in variables [CHECKLIST ITEM #2]; you should define something like $color-header-border in src/styles/variables.scss and use that variable wherever this border color appears. Second, the catalog padding requirement is not fully met: .catalog correctly has padding: 50px 40px;, but .catalog__grid adds padding: 30px; (see src/styles/_catalog.scss and related compiled styles), which changes the effective visible padding of the cards container and violates the spec that the catalog container must have fixed paddings of 50px vertically and 40px horizontally. Please remove or adjust the inner grid padding so that the visible catalog container padding remains exactly 50px 40px as required. Aside from these issues, your semantic structure, BEM/SCSS usage, grid layout, hover effects, and data-qa attributes look solid, so once you fix the color variable and catalog padding, your solution should be ready to pass.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| @@ -1,27 +1,240 @@ | |||
| <!doctype html> | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
There was a problem hiding this comment.
This line uses a hard-coded special color #e5e5e5, which conflicts with checklist item #2: "Repeated sizes and special colors are put to variables". Consider adding a variable for this color in variables.scss and using that variable here.
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" | ||
| /> | ||
| <title>Moyo Catalog</title> |
There was a problem hiding this comment.
The description requires the catalog (cards container) to have fixed paddings 50px vertically and 40px horizontally. Adding padding: 30px; on the inner grid changes the effective padding of the container, so this violates that requirement. Consider removing this padding or adjusting the structure so the catalog container’s visible padding remains 50/40 as specified.
| @@ -0,0 +1,92 @@ | |||
| .card { | |||
| width: $card-width; | |||
There was a problem hiding this comment.
The header border color #e5e5e5 is a special color that should be moved into a SCSS variable, per checklist item: "Repeated sizes and special colors are put to variables." Consider adding it to variables.scss and using that variable here.
| transition: transform $transition; | ||
|
|
||
| &__image { | ||
| width: 100%; |
There was a problem hiding this comment.
catalog is defined as the container with padding 50px 40px, but catalog__grid adds an extra padding: 30px;, which changes the effective container padding. This conflicts with the requirement: "cards container(catalog) have fixed paddings (50px vertically and 40px horizontally)." Remove or adjust this inner padding so the catalog container’s padding matches the spec.
| @@ -0,0 +1,24 @@ | |||
| .catalog { | |||
| padding: 50px 40px; | |||
There was a problem hiding this comment.
In _header.scss, the border color #e5e5e5 is a special color but not stored in a SCSS variable, which violates checklist item: Repeated sizes and special colors are put to variables. Consider adding a variable (e.g. $color-border) in variables.scss and using it here.
| &__grid { | ||
| display: grid; | ||
| gap: 46px 48px; | ||
| padding: 30px; |
There was a problem hiding this comment.
In _catalog.scss, the inner grid has padding: 30px, while the requirement says cards container(catalog) have fixed paddings (50px vertically and 40px horizontally). Because .catalog already has padding: 50px 40px, this extra grid padding contradicts the specification; update or remove it so the catalog container padding matches the description.
| .nav { | ||
| &__list { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 40px; | ||
| list-style: none; | ||
| margin: 0; | ||
| padding: 0; |
There was a problem hiding this comment.
To fully satisfy checklist item "Repeated sizes and special colors are put to variables", consider adding a variable for the header border color (currently hardcoded as #e5e5e5 in _header.scss) here, e.g. $color-header-border, and then use that variable in the header styles.
| @@ -1,3 +1,12 @@ | |||
| @import 'variables'; | |||
|
|
|||
There was a problem hiding this comment.
The use of #e5e5e5 as a raw hex color violates checklist item #2: "Repeated sizes and special colors are put to variables". Move this color into variables.scss (e.g. $color-header-border) and reference the variable here instead of the hardcoded value.
| background: $color-bg; | ||
| } | ||
|
|
||
| @import 'header'; |
There was a problem hiding this comment.
catalog already has padding: 50px 40px; to satisfy the requirement that "cards container(catalog) have fixed paddings (50px vertically and 40px horizontally)". Adding padding: 30px; on .catalog__grid changes the effective container padding and conflicts with that requirement. Remove or adjust this inner padding so the catalog container’s visible padding remains 50/40 as specified.
| @@ -0,0 +1,8 @@ | |||
| $color-primary: #00acdc; | |||
| $color-dark: #060b35; | |||
There was a problem hiding this comment.
Using the literal color #e5e5e5 here breaks checklist item: "Repeated sizes and special colors are put to variables". Consider defining a SCSS variable for this border color in variables.scss and using that variable instead.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it fully implements the catalog page as described: header, navigation, and main catalog are correctly structured with semantic HTML, and BEM + SCSS are consistently used for header, nav, card, and the stars section. All required checklist items are satisfied, including the use of SCSS variables for special colors and repeated sizes (such as the header border color, card width, and 300ms transition), the CSS Grid layout with 1/2/3/4 responsive columns, and the exact catalog padding of 50px 40px without extra inner padding. Interactive behavior matches the specification: cards scale by 20% on hover without breaking the grid, card titles change to the required hover color, nav links use the correct default and hover colors, and the Buy button hover state swaps background and text colors smoothly over the shared transition. The required data-qa attributes (nav-hover, card, card-hover) and the is-active class on the first nav link are correctly applied, and the catalog is wrapped in <main> as required. The failing BackstopJS visual test in CI is due to missing committed reference images, not a defect in your implementation, so it should not block this task. Nice work tightening up the layout, variables, and hover effects to align with all the functional and checklist requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
DEMO LINK
TEST REPORT LINK
All components follow BEM and use SCSS
Repeated sizes and special colors are put to variables
Grid is used for the columns
Cards are shown in 1, 2, 3, or 4 columns based on screen resolution
All changes on
:hoverare smoothCode follows all the Code Style Rules ❗️